🔴 Critical: Overly broad CoW flag condition (Flagged by:3/3)

RW_FLAGS = PTE_PRESENT | PTE_RW | PTE_ACCESSED = 0x23. The expression flags & RW_FLAGS & !PTE_COW simplifies to just flags & RW_FLAGS because PTE_COW (bit 9) doesn't overlap with RW_FLAGS. So flags & 0x23 != 0 is true for any present page (since PRESENT is bit 0 and every present PTE has it set).

This means every present page — including genuinely read-only .text/.rodata — gets marked CoW. Read-only pages don't need CoW; they can be shared directly.

Suggested fix — check for PAGE_RW specifically:

if (flags & PAGE_RW as u32) != 0 || (flags & PTE_COW) != 0 {

🔴 Critical: PD roots bookkeeping may overlap exception stack (Flagged by: 2/3)

SCRATCH_TOP_EXN_STACK_OFFSET = 0x20 means the exception stack top lives here and grows downward. PD_ROOTS_COUNT_OFFSET = 0x28 is only 8 bytes below the exception stack top.

If any i686 guest ever uses exceptions that push state to this stack, the first push would land at offset 0x28 — exactly on top of PD_ROOTS_COUNT. This would corrupt the PD roots count, causing the host to read garbage PD root GPAs.

Suggested mitigations:

• Move PD roots to a dedicated area well below the exception stack (e.g. offset 0x200+)
• Or document the invariant explicitly with a compile-time check that the exception stack size is bounded
• Or reserve explicit space between the exception stack and PD roots bookkeeping

🔴 Critical: Zero test coverage on i686 code path (Flagged by: 3/3)

All existing tests are #[cfg(not(feature = "i686-guest"))]. This PR adds ~500 lines of complex page table manipulation, CoW map building, and snapshot compaction with zero test coverage. This is security-critical VMM code.

Functions without tests:

• build_cow_map() — walks guest-controlled page tables
• compact_i686_snapshot() — 100+ lines of compaction logic
• build_initial_i686_page_tables() — initial PT setup
• i686_pt::Builder — all methods
• virt_to_phys_all() — the i686 PT walker
• read_pd_roots_from_scratch() — reads guest-controlled data

At minimum, unit tests should cover: the i686_pt::Builder, the CoW map builder with crafted PTs, compaction with multiple PD roots, and adversarial inputs (e.g. PD entries pointing out of bounds).

I've added tests.

🟡 Warning: Silent fallback on error (Flagged by: 2/3)

get_memory_size().unwrap_or(0) silently uses 0 on error, which means the va < mem_size check will reject ALL mappings and the CoW map will be empty. This could cause incorrect snapshot behavior (no CoW pages detected). The error should be propagated, or at minimum logged.

This should be resolved now.

🟡 Warning: Fallback returns VA instead of PA (Flagged by: 1/3)

Returning va (a virtual address) when the PD walk fails mixes virtual and physical addresses. The caller uses this result to get a physical page table GPA. If the PD doesn't have an entry for a user PT, this returns the un-translated address, which could cause the host to read from an incorrect location when rebuilding user page tables.

Consider returning Option<u64> and skipping the PT rebuild when resolution fails.

🟡 Warning: Magic number (Flagged by: 2/3)

The x86_64 path uses named constants (CR4_PAE, CR4_OSFXSR, etc.). The i686 path should define CR0_PE, CR0_ET, CR0_WP, CR0_PG constants similarly for self-documenting code:

const CR0_PE: u64 = 1;
const CR0_ET: u64 = 1 << 4;
const CR0_WP: u64 = 1 << 16;
const CR0_PG: u64 = 1 << 31;
cr0: CR0_PE | CR0_ET | CR0_WP | CR0_PG,

🟡 Warning: unshared_snapshot_mem no longer gated on i686 (Flagged by: 1/3)

Old code: unshared_snapshot_mem: { any(feature = "nanvix-unstable", feature = "gdb") }. Now only gated on gdb. The separate_pt_bytes approach may avoid this need, but this should be explicitly validated — if any i686 code path writes to snapshot memory, it will silently fail with a read-only mapping.

This should be resolved now.

I think it would make more sense to have the i686/vmem.rs module used when feature = i686-guest---we should understand that the arch in hyperlight_common is always about the guest arch.

Is there a reason this doesn't match the API of the amd64 stuff so that it can be re-exported from the architecture-independent module, like we do on other platforms?

This whole base va sliding is a hack because of some executables we used to have + the lack of virtual memory in the guest. I don't think that any of the usual guests end up computing anything other than 0 here, so I was thinking it was worth getting rid of entirely. Do the nanvix binaries actually end up with something here? If so, can we replace the unpleasant semantics-breaking relocation with just changing the initial page tables slightly to get things mapped to the VAs that they ask for?

(Not relevant for merging this PR---just curious about answers to these questions for the future).

I would have expected that with the description of this PR, we could get rid of this and use ReadonlySharedMemory unconditionally, since Nanvix will no longer depend on writing to the snapshot shared memory. Is that not true?

Does this need to make the same differentiation between the i686 and x86-64 sources of the initial page table information?

I am still not a huge fan of this extra table.

If the roots are only updated in this table, how does kernel code notice that a snapshot+restore cycle has happened and update its internal source-of-truth for them? (i.e. I assume there is another copy of these hanging out in various struct task-like structures that need to be updated). If the location of those internal-source-of-truth copies of things could be provided by a hypercall or a root finder callback, it would perhaps simplify things, as well as removing this statically-allocated limit to the number of distinct address spaces in the guest.

If this were a hypercall or a callback into the embedder, it would also avoid this architecture-specific dependency.

Why is this information necessary beyond the scope of one address space traversal? And, why is this a useful form of this information?

This can just use mem::size_of::<hyperlight_common::vmem::PageTableEntry>(). Or, if it can't because of some limitation on the const-ness of that expression, it should just be another constant and/or type synonym defined in the same place.

There should ideally be little-to-no arch-specific code in this module: the hyperlight_common::vmem and related APIs should abstract over the architecture-of-the-guest differences well enough that this code can be more-or-less entirely generic. (If there are problems with the vmem api assuming to much, we should fix them, rather than introduce more #[cfg] here).

Again, why isn't this just hyperlight_common/arch/i686/vmem.rs and following the same API as the amd64 (and soon to be aarch64) code?